ci: key TPC dataset caches on pinned generators by ErikBPF · Pull Request #6112 · apache/datafusion-comet

ErikBPF · 2026-09-22T12:25:41Z

Closes #6102.

The TPC-H and TPC-DS dataset caches were keyed on
hashFiles('.github/workflows/pr_build_linux.yml'), so any workflow edit
regenerated both datasets and left entries that a later run can never restore.
The generators were also unpinned: GenTPCHData cloned tpch-dbgen HEAD and
the tpcds-kit checkout had no ref:.

This pins both generators and keys the datasets on their inputs:

TPC-H key is the hash of GenTPCHData.scala; the generator checks out a fixed
tpch-dbgen commit before applying the stdout patch.
TPC-DS key embeds the pinned tpcds-kit commit, which the checkout names
explicitly.
Both caches are split into actions/cache/restore plus a save step guarded to
main, matching the cache-write policy used for other large caches.

check_tpc_dataset_caches() in dev/ci/check-ci-config.py asserts the key
shape, the pinned ref:, and that the TPC caches are no longer bare
actions/cache@vN.

Testing: the new guard fails on the previous revision with five findings and
passes after the change (CI config checks passed); dev/ci/check-suites.py
also passes.

The TPC-H and TPC-DS dataset caches were keyed on the hash of pr_build_linux.yml, so every workflow edit regenerated both datasets and left useless entries that evict main's. The generators were also unpinned: GenTPCHData cloned tpch-dbgen HEAD and the tpcds-kit checkout had no ref, so a generator change could not rotate a dataset key. Pin both generators and key the datasets on their inputs instead: - TPC-H key is the hash of GenTPCHData.scala; the generator now checks out a fixed tpch-dbgen commit before applying the stdout patch. - TPC-DS key embeds the pinned tpcds-kit commit, which the checkout now names explicitly. Save both from main only, matching the cache-write policy for other large caches. check_tpc_dataset_caches() in dev/ci/check-ci-config.py asserts all of the above so the old shape cannot come back.

sunchao

Summary

Prior state and problem: Workflow edits invalidated both TPC datasets, and generator revisions were not fully pinned.
Design approach: Pin both generators, key datasets on generator inputs, and save caches only from main.
Correctness / compatibility analysis: Both pins match current upstream heads. Checked Spark’s generator sources for supported versions 3.4.3, 3.5.9, 4.0.4, 4.1.3, and 4.2.0. No introduced P1/P2 issues found within this review.
Key design decisions: Explicit restore/save steps fit the existing cache policy. The change adds no query-runtime overhead or new abstraction layer.
Implementation sketch: Updates two workflow caches, pins GenTPCHData before applying its existing stdout patch, and adds configuration checks.
Behavioral changes worth calling out: Unrelated workflow edits retain cached data. PR and queue runs restore caches but regenerate on misses without saving.
Suggested improvements: None meeting the P1/P2 reporting threshold.

Reviewed the full three-file diff from 9a4d5f28368d4fbb54ac23c14c5d52d5637ce26d to fb6260914e170276ec0bd87d105bbafdbfcc572e. Confirmed non-draft status. Read the discussion snapshot and live reviews/comments/threads, all empty. Routed skill: review-comet-pr; no sibling skill applies.

Exact-head CI: label passed. Comet CI, CodeQL, and Check PR Title report action_required. Comet CI has no executed jobs, so there is no build/test verdict.

Local validation passed: CI configuration checks, suite registration, workflow actionlint, and the new guard’s five expected failures against base fixtures. Both pinned data generators compiled, and bounded SF=1 stdout tests produced expected row counts. Full Maven/native builds, Spark Parquet generation, benchmark query suites, and hosted cache restore/save behavior were not executed. The project working tree remains unchanged.

andygrove · 2026-09-28T18:37:48Z

This is a light fully automated review since there are so many PRs open.

The restore/save split matches the existing Maven cache pattern well.

.github/workflows/README.md:396-399 still describes the setup this PR replaces. It says the TPC-H and TPC-DS dataset caches "keep the read-write form and are out of scope entirely" and that they are "keyed on this workflow file". After this change they use actions/cache/restore plus a main-only actions/cache/save, the same pattern the section documents for the Maven and cargo caches just above, and they are keyed on GenTPCHData.scala and the pinned tpcds-kit commit. Could this paragraph be updated to match what pr_build_linux.yml does now?

The new keys at pr_build_linux.yml:707 and :792 also no longer cover the generator arguments. --scaleFactor 1 --numPartitions 1 is passed at :716 and :816 and lives only in the workflow file, which the old key hashed. If a later change bumps --scaleFactor without touching GenTPCHData.scala or the tpcds-kit ref, the cache still hits, the generate step is skipped, and the queries run against the old dataset. Would it make sense to fold the scale factor and partition count into the keys?

The TPC-H and TPC-DS dataset caches were keyed on the hash of pr_build_linux.yml, so every workflow edit regenerated both datasets and left useless entries that evict main's. The generators were also unpinned: GenTPCHData cloned tpch-dbgen HEAD and the tpcds-kit checkout had no ref, so a generator change could not rotate a dataset key. Pin both generators and key the datasets on their inputs instead: - TPC-H key is the hash of GenTPCHData.scala; the generator now checks out a fixed tpch-dbgen commit before applying the stdout patch. - TPC-DS key embeds the pinned tpcds-kit commit, which the checkout now names explicitly. Save both from main only, matching the cache-write policy for other large caches. check_tpc_dataset_caches() in dev/ci/check-ci-config.py asserts all of the above so the old shape cannot come back.

Keep the verified refreshed tree while retaining the published head as an ancestor, allowing a normal push without rewriting history.

ErikBPF · 2026-10-02T11:26:56Z

Updated against pinned main 63fd1c9e and addressed the cache-input/documentation feedback. Published ancestry is retained with an identical-tree merge, without force-pushing.

Scale factor and partition count now come from the same job inputs used by actual generator arguments, restore/save cache keys, and dataset/query paths. Pinned generators, exact-match dataset restores, main-only saves and upstream Cargo policy are retained. The workflow README now describes this behavior accurately.

Verification on Apollo:

The extended guard failed on the rebased pre-fix workflow with ten expected coupling diagnostics, then passed after the fix.
15 input/mutation checks passed, along with CI/suite guards, formatting and actionlint.
The Maven reactor and RAT checks passed; Maven tests were explicitly skipped.
Actual Maven/Spark TPC-H region generation produced one Parquet file containing five rows. Pinned native stdout checks produced five region and 25 nation rows.
Independent source review passed; the final patch exactly matches the reviewed snapshot.

Limitations: the local TPC-H build used gcc -std=gnu17, so this does not establish literal hosted compiler parity. TPC-DS native build remains blocked at print.c:235 by -Werror=format-security after three repairs; no hardening/assertions were weakened. Full datasets, query suites, TPC-DS Spark generation and hosted cache behavior were not verified.

Updated head: f087cf42c3367cec43ceb54b20643462a93c469a (tested tree unchanged from 7f68df22fe93635abce40c5ca66052c9879bcf23).

TPC-DS follow-up

TPC-DS follow-up: a sandbox-only fprintf → fputs fix retained compiler hardening. Native generation and Spark conversion passed: 35 rows, one Parquet file. Independent review passed.

PR source remains unchanged. Local generator patch and compiler compatibility flags were required; unmodified hosted-workflow compatibility and full TPC-DS/query suites remain unverified.

Current-main diff correction

Both intended commits were rebased onto ca1c143a. An ordinary merge of actual main preserved public ancestry without force-pushing and produced exactly the clean rebased tree. GitHub Files Changed is now verified at 4 files, +173/-22: workflow README, Linux workflow, CI guard and the existing GenTPCHData.scala change. No sandbox generator fix was added to the PR.

Current-base input/mutation checks (15), CI/suite guards, benchmark checker (55 suites), inventory/summary tests and workflow formatting checks passed. Previous TPC-H and patched-local TPC-DS runtime evidence remains tied to the earlier source/base and its recorded compatibility overrides, not a new-base runtime or hosted-cache claim.

Updated head: a21349caf3ce8570256a8e42d5f3b3fd57d6d032.

ErikBPF · 2026-10-11T20:14:52Z

Rechecked the current head (a21349c). Both requested changes are included: the README matches the restore/main-only-save behavior, and the scale factor and partition count are shared by the generator arguments, dataset paths, and matching restore/save keys.

Fresh runs of dev/ci/check-ci-config.py, dev/ci/check-suites.py, and dev/ci/check-benchmark-runner.py passed.

The current Comet CI and CodeQL runs are awaiting approval, so there is no current-head hosted build/test verdict yet. Could a maintainer approve those runs?

Preserve generator-keyed TPC caches alongside upstream suite removals and CI routing changes without rewriting the PR history. Refs apache#6112

ErikBPF · 2026-10-13T14:45:18Z

Merged current main (474b5ece8) into this branch without conflicts or history rewriting. Updated head: ab4edd002feafcc44ae63362db1aa667899564ac; the PR remains scoped to the same four files (+173/-22), including the generator-input coupling and main-only cache saves.

Fresh local checks passed: CI configuration, suite inventory, versioned-source parity, benchmark runner (60 suites), Iceberg shard inventory (15 tests), and write-summary checks (6 tests). This is static/configuration validation, not a fresh TPC generation, cache-runtime, or hosted test result.

The Comet CI and CodeQL runs for this exact head require maintainer approval. Could a maintainer approve them?

…0261013

…hange

ErikBPF · 2026-10-26T21:59:53Z

Updated head: fd0b0e68b (documentation and comment corrections only).

Validated on Apollo against current main (c4a0b1f2b):

python3 dev/ci/check-ci-config.py: passed.
actionlint --shellcheck=off, the CI invocation: passed.
Three remaining guard gaps reproduced by mutation, for the record: dataset path: / SPARK_TPCH_DATA / SPARK_TPCDS_DATA drift still passes; setting dbgenRevision to master still passes; renaming the restore step and adding restore-keys still passes. Tell us if you want those tightened in this PR.

Not rerun: actual TPC generation and hosted cache behavior.

CI and CodeQL are still action_required on this head. Could a maintainer approve them?

comphead

Exact-key restores with main-only saves and no restore-keys mean a pull request or fork cannot restore a stale dataset or poison the cache, and both generator pins match the current upstream heads. One comment on what the TPC-H key covers. Separately, this conflicts with #6113 in dev/ci/check-ci-config.py, where both append a check after check_cache_save_scope, so whichever lands second needs a small rebase.

comphead · 2026-10-29T20:22:01Z

        with:
          path: ./tpch
-          key: tpch-${{ hashFiles('.github/workflows/pr_build_linux.yml') }}
+          key: tpch-sf${{ env.TPC_SCALE_FACTOR }}-p${{ env.TPC_NUM_PARTITIONS }}-${{ hashFiles('spark/src/test/scala/org/apache/spark/sql/GenTPCHData.scala') }}


The key hashes only GenTPCHData.scala, but the generated data also depends on TPCH.scala, which holds Dbgen (it builds the ./dbgen -q ... -T <table> -s <sf> command line) and TPCHTables (the schemas). A change there would still hit the cache and run the queries on the old dataset, which is the failure this PR removes. Could the key hash TPCH.scala too, here and in the save key at line 796? For TPC-DS, GenTPCDSData and TPCDSTables come from Spark's spark-sql tests jar at the default profile's spark.version, so a Spark bump does not rotate that key either. Is that intended?

…0261013 # Conflicts: # dev/ci/check-ci-config.py

ErikBPF · 2026-11-11T15:00:41Z

Merged current main (e59d93730) into this branch at head 9685a2e10 and revalidated it against the CI restructuring that landed after the review: #7311 (fork PR tier), #7516 (Spark SQL 4.1 / Iceberg 1.11 on every PR) and #7521 (Spark 3.4 removal).

The only conflict was dev/ci/check-ci-config.py, where both sides appended new checks. The TPC assertions keep their original intent: exact-key restores, saves only on main, no restore-keys, both generators pinned, and scale factor/partition count shared between generator arguments, dataset paths and keys.

Verification on the merged head: python3 dev/ci/check-ci-config.py passed, dev/ci/test-fork-push-changed-files.py (7 tests) and dev/ci/test-fork-ci-status.cjs passed. A mutation that puts hashFiles(pr_build_linux.yml) back into the TPC-H key still fails the check, so the assertion is not vacuous.

GitHub now reports the PR mergeable with the approval retained. Ready for re-review.

github-actions Bot added build Build environment enhancement New feature or request labels Sep 22, 2026

sunchao approved these changes Sep 25, 2026

View reviewed changes

andygrove mentioned this pull request Sep 29, 2026

ci: stop main's cargo build cache from being evicted #6387

Closed

ErikBPF added 3 commits October 1, 2026 22:35

ci: couple TPC cache keys to generation inputs

7f68df2

chore: preserve published branch ancestry

f087cf4

Keep the verified refreshed tree while retaining the published head as an ancestor, allowing a normal push without rewriting history.

chore: merge main for a scoped PR comparison

a21349c

andygrove mentioned this pull request Oct 11, 2026

ci: retry the scalafix lint when Maven cannot resolve the plugin #6061

Merged

chore(ci): merge main to keep PR checks current

ab4edd0

Preserve generator-keyed TPC caches alongside upstream suite removals and CI routing changes without rewriting the PR history. Refs apache#6112

ErikBPF added 2 commits October 25, 2026 18:53

Merge remote-tracking branch 'origin/main' into maintenance/pr-6112-2…

38c622f

…0261013

docs(ci): correct TPC cache key comments after the pinned-generator c…

fd0b0e6

…hange

comphead reviewed Oct 29, 2026

View reviewed changes

Merge remote-tracking branch 'origin/main' into maintenance/pr-6112-2…

9685a2e

…0261013 # Conflicts: # dev/ci/check-ci-config.py

Navigation Menu

Repository navigation

ci: key TPC dataset caches on pinned generators - #6112

ci: key TPC dataset caches on pinned generators#6112
ErikBPF wants to merge 9 commits into
apache:mainfrom
ErikBPF:ci/tpc-cache-key

ErikBPF commented Sep 22, 2026

Uh oh!

sunchao left a comment

Uh oh!

andygrove commented Sep 28, 2026

Uh oh!

ErikBPF commented Oct 2, 2026 •

edited

Loading

Uh oh!

ErikBPF commented Oct 11, 2026

Uh oh!

ErikBPF commented Oct 13, 2026

Uh oh!

ErikBPF commented Oct 26, 2026

Uh oh!

comphead left a comment

Uh oh!

comphead Oct 29, 2026

Uh oh!

ErikBPF commented Nov 11, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

ErikBPF commented Sep 22, 2026

Uh oh!

sunchao left a comment

Choose a reason for hiding this comment

Uh oh!

andygrove commented Sep 28, 2026

Uh oh!

ErikBPF commented Oct 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TPC-DS follow-up

Current-main diff correction

Uh oh!

ErikBPF commented Oct 11, 2026

Uh oh!

ErikBPF commented Oct 13, 2026

Uh oh!

ErikBPF commented Oct 26, 2026

Uh oh!

comphead left a comment

Choose a reason for hiding this comment

Uh oh!

comphead Oct 29, 2026

Choose a reason for hiding this comment

Uh oh!

ErikBPF commented Nov 11, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ErikBPF commented Oct 2, 2026 •

edited

Loading